feat(media): a converter receives a source handle, never the file's bytes (D104) - #344
Open
fazpu wants to merge 3 commits into
Open
feat(media): a converter receives a source handle, never the file's bytes (D104)#344fazpu wants to merge 3 commits into
fazpu wants to merge 3 commits into
Conversation
fazpu
force-pushed
the
feat/video-source-handle
branch
from
September 1, 2026 00:04
bcee1ad to
726c79d
Compare
fazpu
force-pushed
the
feat/video-source-handle
branch
from
September 1, 2026 00:05
726c79d to
f26b763
Compare
…ytes D104, amending D65's convert(bytes, mime, hints). Design: media_design.md §2.1. A text file fits in memory and a video does not. Under the bytes-first contract the client, the HTTP process, the object store and the converter each materialize a complete copy of the same file, so two concurrent large ingests can exhaust a worker that would have processed either one alone. That is an availability property, not an optimisation. Adds SourceHandlePort with exactly three reads and deliberately no fourth: stream in bounded chunks, read a half-open range [start, end), or materialize to a temporary local file for decoders like FFmpeg that need a seekable path. There is no operation returning the whole source as bytes -- the absence is the load-bearing part: a contract that offers buffering as the convenient option gets buffering, and the failure surfaces on the first large file in production rather than in review. ObjectStorePort grows open_stream and read_range so the handle has something to sit on; read_bytes stays for the small text objects every shipped route converts. Implemented for both the local filesystem and MinIO adapters -- S3 ranges are inclusive on both ends, so the exclusive end becomes end-1 on the wire and every interval in the system stays half-open (D65). Bounds live in materialization, since that is the operation that can exhaust a host: - the caller declares what it can afford and an oversized source is refused before any byte moves -- one integer comparison, not a filled disk; - written bytes are verified against the recorded content hash. An immutable object cannot legitimately change, so a mismatch is corruption or a wrong key, never retryable. Without the check a truncated read becomes a short document that looks complete -- the silent-loss failure D65's coverage rules exist to prevent; - the recorded size is a claim, not a guarantee, so the streaming write also counts what it writes and refuses a source exceeding both the declaration and the accepted bound; and - the temporary file's lifetime belongs to the handle, so a route failing mid-decode cannot leak the file it asked for. One contract serves both deployment shapes: a self-host directory tree and a cloud object store present the same three operations, so no route branches on where bytes live. Nothing consumes the handle yet -- no converter signature changes in this commit and no route behaviour changes. Extending ObjectStorePort required adding the two reads to three test fakes and raising the port-inventory count to thirteen. Validation: ruff format and check clean; pyright 0 errors; 1303 passed, 685 skipped. One pre-existing failure, test_sdk_pushes_lineage_metadata_to_e0, reproduces on the base commit 7c41fdb without these changes -- mimetypes on this machine does not resolve .md, so the SDK guesses application/octet-stream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTv3baR8R5EgEGFePkQMjK
…e read External review returned BLOCK with eight findings. All addressed. BLOCK — teardown used unlink plus rmdir, which raises "directory not empty" the moment a decoder writes anything beside the source it was handed: an index, a sidecar, a split audio track. That both leaked the directory and masked whatever the route was already raising. Now shutil.rmtree with ignore_errors, so cleanup cannot become the failure. BLOCK — reads could come back short. LocalFS returned a truncated slice past EOF, MinIO let botocore's ClientError escape on a 416 (a provider type in a port D61 keeps provider-free) and returned a short 206 body otherwise, and the handle validated only against the recorded size, never against what actually arrived. A stored object shorter than its record therefore produced a silent short read. All three now return exactly the requested bytes or raise SourceRangeError, and 416 is translated at the adapter. This is the failure the range contract exists to prevent: a container parser handed a truncated header cannot distinguish it from a valid one and produces confident nonsense instead of an error. MAJOR — max_bytes had to be positive, so a zero-byte source could not be materialized under a zero bound; a negative bound now raises ValueError as the caller bug it is, rather than being reported as a size refusal. MAJOR — MinIO's open_stream and read_range had no tests at all. The fakes were updated but nothing exercised the wire translation, so an off-by-one in the inclusive-range conversion would have passed silently. Five tests added, including one asserting [2, 5) reaches S3 as bytes=2-4. MINOR — the materialized file is named with the source's own suffix. Demuxers sniff the container from the extension before falling back to probing, so a bare "source" costs a class of cannot-open bug for nothing. MINOR — added read_bounded(max_bytes=...). The review's point stands: with no in-memory read at all, a text route writes b"".join(open_stream()), which is an unbounded read wearing a bounded read's clothes. The design intent was never "no bytes in memory" but "no bytes in memory without someone deciding how many", so the fourth read requires the caller to name a limit. Design and D104 corrected — they claimed three reads and no fourth, which is no longer true. MINOR — tests added for zero-byte sources, temp_root=None, cleanup with a sidecar present, a truncated stored object, and two concurrent live materializations not sharing or co-deleting a directory. Validation: ruff clean; pyright 0 errors; test inventory OK; 1319 passed, 687 skipped. The one failure, test_sdk_pushes_lineage_metadata_to_e0, reproduces on the base commit without these changes — mimetypes on this machine does not resolve .md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTv3baR8R5EgEGFePkQMjK
… truth Second independent review (Codex) converged with the first on both BLOCKs already fixed, and found five more. All addressed. The sharpest one was aimed at my own claim. D104 said there is no whole-file read, but read_range(0, byte_size) is exactly that with extra steps, and nothing capped a range or a chunk. The claim was ergonomic friction dressed as an availability invariant. A handle now takes an optional max_read_bytes ceiling that refuses any single range, chunk, or bounded read above it, and the design says plainly that without a ceiling the property is friction, not a guarantee. Overclaiming in a decision doc is worse than the gap it hid. open_stream returned a bare Iterator, so a caller that stopped early held a file descriptor or an HTTP body until garbage collection. GC timing is not a resource-lifetime contract and enough abandoned reads exhaust the descriptor limit or the connection pool. It is now a context manager at every level -- port, both adapters, handle, and the three fakes -- released when the block exits. The streaming guard fired only after a full default chunk had moved: a 100-byte allowance could pull a megabyte before noticing. The read is now sized to the remaining allowance plus one byte, the extra byte being what makes "over" detectable at all -- a read that exactly fills the allowance is indistinguishable from one that stops there. Materialization verified the hash but never the length, so bytes that hash correctly could still be described by a wrong size: a source recorded as 10 bytes materialized happily at 5000 while a range read past 10 was refused -- two access paths disagreeing about one source. Now a typed SourceSizeMismatchError, distinct from the hash error because they catch different lies. SourceIdentity carried a mime field while the converter contract already takes mime as its own argument. Two authorities that can disagree, and a route would have to know which one routing used. Dropped from the identity. Concurrent materializations still have no aggregate temp-disk budget -- two 6 GiB copies each pass their own bound and together need 12 GiB. That is worker-level admission, not a handle concern, so it is documented as a stated boundary in both the design and D104 rather than quietly left. Tests: two asserted less than their names claimed. "Refused before any bytes move" checked only that no file survived, and the understated-size test read its whole 5000-byte source before raising and still passed. Both now use a counting store and assert bytes actually served (0, and <=101). Added coverage for the ceiling, the size mismatch, and releasing a stream abandoned after one chunk. Validation: ruff clean; pyright 0 errors; test inventory OK; 1322 passed, 687 skipped. The single failure, test_sdk_pushes_lineage_metadata_to_e0, reproduces on the base commit without these changes -- mimetypes on this machine does not resolve .md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XTv3baR8R5EgEGFePkQMjK
fazpu
force-pushed
the
feat/video-source-handle
branch
from
September 1, 2026 00:36
093c747 to
24472a1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A text file fits in memory and a video does not. Under D65's
convert(bytes, mime, hints)contract, the client, the HTTP process, the object store, and the converter each materialize a complete copy of the same file. Two concurrent large ingests can exhaust a worker that would have processed either one alone. That is an availability property, not an optimisation — and it is the reason no media route can be built on the current contract.Scope
SourceHandlePort— bounded reads of one immutable, already-hashed source;ObjectSourceHandle— the adapter over anyObjectStorePort;open_stream/read_rangeonObjectStorePort, implemented for the local filesystem and MinIO;decisions.md, design inplan/designs/media_design.md§2.1.Nothing consumes the handle yet. No converter signature changes here and no route behaviour changes. This is the seam a media route needs to exist at all.
Important decisions
Four reads, every one of them bounded. Stream in bounded chunks; read a half-open range
[start, end); materialize to a temporary local file for decoders like FFmpeg that need a seekable path; or read the whole thing into memory after naming the limit. There is no operation that reads a source of unknown size — the hazard was never bytes in memory, it was bytes in memory without anyone having decided how many.A read ceiling, because naming a large limit is still naming one.
read_range(0, byte_size)is a whole-file read with extra steps, so a handle can carrymax_read_bytesand refuse any single read above it. Without a ceiling the property is ergonomic friction; with one it is enforced. The design says which it is rather than overclaiming.A stream is a resource, not just bytes. Underneath is a file descriptor or an HTTP body, so
open_streamis a context manager released at block exit. A bare iterator would leave an early-stopping caller holding it until garbage collection, and GC timing is not a lifetime contract.Reads never come back short, and length is verified alongside hash. A range returns exactly what was asked for or raises — a parser handed a truncated header cannot tell it from a valid one. Bytes can also hash correctly while being described by a wrong size, which would let materialization and range reads disagree about the length of one source.
Half-open ranges match every other interval in the system (D65 locators), so nobody has to remember which end is inclusive. S3 ranges are inclusive on both ends, so the exclusive
endbecomesend - 1on the wire — the conversion lives in the adapter so the port stays consistent regardless of provider.Bounds live in materialization, since that is the operation that can actually exhaust a host:
One contract, both deployment shapes. A self-host directory tree and a cloud object store present the same three operations, so no route branches on where bytes live.
Sources inspected
Base
origin/main7c41fdb8. D38 (router), D57 (Markdown coordinate system), D61 (port boundary), D65 +plan/designs/media_design.md§2 (media routes and the converter contract).read_bytesis retained deliberately — every shipped route converts small text and has no reason to stream.Review
Double-reviewed by two independent reviewers. Both returned BLOCK and converged on the same two blocking defects; the second found five more, including one aimed squarely at this PR's own claim.
Round 1 — teardown used
unlink+rmdir, which raises directory not empty the moment a decoder writes a sidecar beside the source, leaking the directory and masking the route's own exception. And reads could come back short: LocalFS truncated past EOF, MinIO leakedbotocore.ClientErroron a 416 (a provider type in a port D61 keeps provider-free), and the handle checked only the recorded size, never what actually arrived.Round 2 —
read_range(0, byte_size)made the "no whole-file read" claim false, so the ceiling was added and the docs corrected;open_streamleaked descriptors on abandonment; the streaming guard fired only after a full 1 MiB chunk had moved for a 100-byte allowance; length was never verified against the record;SourceIdentity.mimeduplicated an authority the converter signature already owns. Two tests also asserted less than their names claimed — "refused before any bytes move" only checked that no file survived — and now use a counting store asserting bytes actually served.Concurrent materializations still have no aggregate temp-disk budget. That is worker-level admission, and it is documented as a stated boundary rather than quietly left.
Validation
ruff format --checkandruff checkclean;pyright— 0 errors;pytest— 1303 passed, 685 skipped;test_source_handle.pyandtest_minio_store.py: ordered streaming, half-open ranges, degenerate and past-the-end ranges, cleanup on success, on exception, and with a sidecar present, refusal before any byte is served, the understated-size guard bounded to ≤101 bytes moved, hash mismatch, size mismatch, the read ceiling, zero-byte sources,temp_root=None, concurrent materializations, stream release after abandonment, and the MinIO wire translation asserting[2, 5)reaches S3 asbytes=2-4.One pre-existing failure,
test_sdk_pushes_lineage_metadata_to_e0, reproduces on base commit7c41fdb8with these changes stashed:mimetypeson this machine does not resolve.md, so the SDK guessesapplication/octet-stream. Not introduced here and not fixed here.Extending
ObjectStorePortrequired adding the two reads to three test fakes and raising the port-inventory count to thirteen.Limitations
The handle has no consumer yet — the D65 converter signature still takes bytes, and changing it is the next step rather than part of this PR. The trusted structural probe (container, tracks, codecs) is not here. No video route exists and none is claimed.
Follow-up
🤖 Generated with Claude Code
https://claude.ai/code/session_01XTv3baR8R5EgEGFePkQMjK